Fix attached CSV being shadowed by same-named entity list#6908
Fix attached CSV being shadowed by same-named entity list#6908grzesiek2010 wants to merge 4 commits into
Conversation
175a46c to
9dc716e
Compare
|
At first, the issue seemed pretty straightforward based on the description, but it turned out to be more complex. In Javarosa, choices are loaded without any awareness of entities - it treats all choices the same way. I believe that was intentional, and we don’t want to change this by passing the In Collect, we have Given that, the simplest solution I can think of is to introduce something similar to what we did for enabling/disabling entities in the settings. There, we had a function that determined whether What do you think about this approach @seadowg? |
seadowg
left a comment
There was a problem hiding this comment.
I've realized that I don't think this entirely solves the problem sadly: you could still have a form that uses an entity list, but also has a non entity list CSV that clashes with an entity list in the project. That's definitely an edge case, but I think the flow in the issue is as well.
I think to fully solve this we really need to be able to have access to information about the media file we're referencing (through a MediaFileRepository) for example. This is something that we've talked about on and off, but it's always been hard to justify. Maybe we should bite the bullet here? This could be a good issue to drive that out.
9dc716e to
760c7aa
Compare
760c7aa to
01dbdc1
Compare
|
@seadowg After a long time, I got back to this PR and ended up using a different fix (based on your comment). Feel free to review it. |
Closes #6425
Why is this the best possible solution? Were any other approaches considered?
The core problem is that there is no authoritative "entity list vs. plain CSV" signal that survives to parse time. The only authoritative signal (
type=entityListin the OpenRosa manifest) exists atdownload time. At parse time, both look identical with the same
jr://file-csv/<name>.csvreference, same on-disk CSV - so the fix has to manufacture a reliable discriminator that lives until parsing.Chosen approach:
Two changes that together make file presence in the form's media directory the discriminator between a plain CSV and an entity list:
isSupporteddefers to the attached file.LocalEntitiesInstanceProvidernow returnsfalsewhen the instance'ssrcresolves to a media file that exists on disk, letting JavaRosa serve the attached CSV;Otherwise, it serves the entity list from the DB.
Entity-list seeds are no longer persisted. After import into the DB, the temp CSV is deleted (
tempMediaFile.delete()). This is safe because every entity read is DB-backed, so the seed file is redundant - and with it gone, a CSV in the media dir can only be a genuine attachment.Other approaches considered:
isSupportedonly gets the instance id and rawsrc, which is identical for a CSV and an entity list; JavaRosa has no entity awareness.discriminator.
How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
When a form attaches a static CSV whose name matches an entity list in the project, the attached CSV is now used instead of the entity list (#6425). Genuine entity-list forms are unaffected and keep loading from the DB.
The change touches how external instances are resolved and stops persisting entity-list seed CSVs in the media directory, so testing should focus on entity data loading being correct across the different form scenarios:
Do we need any specific form for testing your changes? If so, please attach one.
The one linked in the issue for reproducing the bug.
Does this change require updates to documentation? If so, please file an issue here and include the link below.
No.
Before submitting this PR, please make sure you have:
./gradlew connectedAndroidTest(or./gradlew testLab) and confirmed all checks still passDateFormatsTest